-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[Support] Enable CRTP for GraphWriter (NFC) #152322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Previously, specializing the `GraphWriter` class required a full class specialization. This change introduces CRTP for `GraphWriter`, allowing for partial specialization. This change is in support of printing the module dependency graph as part of the RFC for driver-managed module builds, for which we want to print the graph nodes in a more human-readable format by: - Printing descriptive IDs instead of pointer addresses as node labels. - Printing the full node labels seperatly from the node relations to avoid clutter. With this approach, only `GraphWriter::writeNodes()` needs to be specialized (, aside from `DOTGraphTraits`). Example output: ```dot digraph "Dependency Graph" { node [shape=Mrecord] edge [dir=back] "Clang Module 'A'" [ label="{ ModuleName | ... }" ] "C++20 Module 'B'" [ label="{ ModuleName | ModuleType | InputFile | ... }" ] ... "C++20 Module 'B'" -> "Clang Module 'A'" ... } ``` RFC for driver-managed module builds: https://discourse.llvm.org/t/rfc-modules-support-simple-c-20-modules-use-from-the-clang-driver-without-a-build-system
@llvm/pr-subscribers-llvm-support Author: Naveen Seth Hanig (naveen-seth) ChangesPreviously, specializing the This change is in support of printing the module dependency graph as part of the RFC for driver-managed module builds, for which we want to print the graph nodes in a more human-readable format by:
With this approach, only Example output: digraph "Dependency Graph" {
node [shape=Mrecord]
edge [dir=back]
"Clang Module 'A'" [ label="{ ModuleName | ... }" ]
"C++20 Module 'B'" [ label="{ ModuleName | ModuleType | InputFile | ... }" ]
...
"C++20 Module 'B'" -> "Clang Module 'A'"
...
} RFC for driver-managed module builds: Full diff: https://github.com/llvm/llvm-project/pull/152322.diff 1 Files Affected:
diff --git a/llvm/include/llvm/Support/GraphWriter.h b/llvm/include/llvm/Support/GraphWriter.h
index 39a4c0befbb89..af2e5016298e6 100644
--- a/llvm/include/llvm/Support/GraphWriter.h
+++ b/llvm/include/llvm/Support/GraphWriter.h
@@ -61,8 +61,7 @@ enum Name {
LLVM_ABI bool DisplayGraph(StringRef Filename, bool wait = true,
GraphProgram::Name program = GraphProgram::DOT);
-template<typename GraphType>
-class GraphWriter {
+template <typename GraphType, typename Derived> class GraphWriterBase {
raw_ostream &O;
const GraphType &G;
bool RenderUsingHTML = false;
@@ -75,9 +74,15 @@ class GraphWriter {
DOTTraits DTraits;
static_assert(std::is_pointer_v<NodeRef>,
- "FIXME: Currently GraphWriter requires the NodeRef type to be "
- "a pointer.\nThe pointer usage should be moved to "
- "DOTGraphTraits, and removed from GraphWriter itself.");
+ "FIXME: Currently GraphWriterBase requires the NodeRef type to "
+ "be a pointer.\nThe pointer usage should be moved to "
+ "DOTGraphTraits, and removed from GraphWriterBase itself.");
+
+ // Cast the 'this' pointer to the derived type and return a reference.
+ Derived &getDerived() { return *static_cast<Derived *>(this); }
+ const Derived &getDerived() const {
+ return *static_cast<const Derived *>(this);
+ }
// Writes the edge labels of the node to O and returns true if there are any
// edge labels not equal to the empty string "".
@@ -118,23 +123,24 @@ class GraphWriter {
}
public:
- GraphWriter(raw_ostream &o, const GraphType &g, bool SN) : O(o), G(g) {
+ GraphWriterBase(raw_ostream &o, const GraphType &g, bool SN) : O(o), G(g) {
DTraits = DOTTraits(SN);
RenderUsingHTML = DTraits.renderNodesUsingHTML();
}
+ virtual ~GraphWriterBase() {}
void writeGraph(const std::string &Title = "") {
// Output the header for the graph...
- writeHeader(Title);
+ getDerived().writeHeader(Title);
// Emit all of the nodes in the graph...
- writeNodes();
+ getDerived().writeNodes();
// Output any customizations on the graph
- DOTGraphTraits<GraphType>::addCustomGraphFeatures(G, *this);
+ DOTGraphTraits<GraphType>::addCustomGraphFeatures(G, getDerived());
// Output the end of the graph
- writeFooter();
+ getDerived().writeFooter();
}
void writeHeader(const std::string &Title) {
@@ -166,8 +172,8 @@ class GraphWriter {
void writeNodes() {
// Loop over the graph, printing it out...
for (const auto Node : nodes<GraphType>(G))
- if (!isNodeHidden(Node))
- writeNode(Node);
+ if (!getDerived().isNodeHidden(Node))
+ getDerived().writeNode(Node);
}
bool isNodeHidden(NodeRef Node) { return DTraits.isNodeHidden(Node, G); }
@@ -302,9 +308,9 @@ class GraphWriter {
if (DTraits.getEdgeSourceLabel(Node, EI).empty())
edgeidx = -1;
- emitEdge(static_cast<const void*>(Node), edgeidx,
- static_cast<const void*>(TargetNode), DestPort,
- DTraits.getEdgeAttributes(Node, EI, G));
+ getDerived().emitEdge(static_cast<const void *>(Node), edgeidx,
+ static_cast<const void *>(TargetNode), DestPort,
+ DTraits.getEdgeAttributes(Node, EI, G));
}
}
@@ -357,10 +363,17 @@ class GraphWriter {
}
};
-template<typename GraphType>
+template <typename GraphType>
+class GraphWriter : public GraphWriterBase<GraphType, GraphWriter<GraphType>> {
+public:
+ GraphWriter(raw_ostream &o, const GraphType &g, bool SN)
+ : GraphWriterBase<GraphType, GraphWriter<GraphType>>(o, g, SN) {}
+ ~GraphWriter() override {}
+};
+
+template <typename GraphType>
raw_ostream &WriteGraph(raw_ostream &O, const GraphType &G,
- bool ShortNames = false,
- const Twine &Title = "") {
+ bool ShortNames = false, const Twine &Title = "") {
// Start the graph emission process...
GraphWriter<GraphType> W(O, G, ShortNames);
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. It's already a template so this shouldn't really add any compile time cost.
Because commit 474bbc1 does not make the base class's members protected, this makes it impossible to partially specialize the base class using CRTP.
Previously, specializing the
GraphWriter
class required a full class specialization.This change introduces CRTP for
GraphWriter
, allowing for partial specialization.This change is in support of printing the module dependency graph as part of the RFC for driver-managed module builds, for which we want to print the graph nodes in a more human-readable format by:
With this approach, only
GraphWriter::writeNodes()
needs to be specialized (, aside fromDOTGraphTraits
).Example output:
RFC for driver-managed module builds:
https://discourse.llvm.org/t/rfc-modules-support-simple-c-20-modules-use-from-the-clang-driver-without-a-build-system